Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update copy on notifications for github services deprecation #5067

Merged

Conversation

agjohnson
Copy link
Contributor

Also adds some documentation.

* Drop "deprecated webhook endpoint" copy, this is core team nomenclature, not user
nomenclature.
* Add small amount of docs to point to
@@ -36,6 +36,8 @@ As an example, the URL pattern looks like this: *readthedocs.org/api/v2/webhook/

Use this URL when setting up a new webhook with your provider -- these steps vary depending on the provider:

.. _webhook-integration-github:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes. I added these as I wanted to point from our UI to these -- meaning the headings can't be allowed to change. Perhaps explicit labels is most helpful here? Also doc comments could warn doc authors though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should only use explicit when we are linking them from another place and avoid adding them "just because" or "just in case we need it later".

That way, we will know that if it's there it's because an explicit reason.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the linter or sphinx itself can catch errors if the title is renamed. And probably is worth adding this to the docs guide https://docs.readthedocs.io/en/latest/docs.html

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is catch automatically, I'd love it! I'm 👍 on setting this up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't turn on the sphinx check yet in the whole project bc of #4166 (comment) what I usually do is just check the warnings locally :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more than I want to tackle now. I'll leave them and we can revisit later to either throw errors or something. I don't think removing is important right now.

@@ -1,7 +1,8 @@
<p>Just a heads up, your project {{ project.name }} has configured a <em>DEPRECATED</em> webhook to trigger new builds and should be upgraded. Projects hitting these deprecated webhook will stop building on Jan 1, 2019. Please, update it soon!</p>
<p>Your project, {{ project.name }}, is currently using GitHub Services to trigger builds on Read the Docs. Effective January 31, 2019, GitHub will no longer process requests using the Services feature, and so Read the Docs will not receive notifications on updates to your repository.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{{ project.name }} is surrounded by two commas, I guess we only need one, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar time! {{ project.name}} would be an appositive, requiring surrounding in commas in this particular use:
http://www.chompchomp.com/terms/appositive.htm

We can avoid commas if we instead write:

Your {{ project.name }} project is currently using ...

But this is slightly awkward as {{ project.name }} could be interpreted as an adjective given the right project name -- for example, for a project named docs:

Your docs project is currently using...

And so this is less clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the link!

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these changes are right.

I left something to consider, but I do really want to merge and deploy this as soon as possible and do not block it anymore. We are running out of time here.

@@ -53,6 +55,8 @@ For a 403 error, it's likely that the Payload URL is incorrect.

.. note:: The webhook token, intended for the GitHub **Secret** field, is not yet implemented.

.. _webhook-integration-bitbucket:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, following my idea from https://github.com/rtfd/readthedocs.org/pull/5067/files#r245598268, this one and the gitlab should be removed to avoid confusions I'd say.


<p>To update the webhook your project is hitting, you need to go to the project's settings under your VCS service (GitHub, Bitbucket or GitLab) and remove the Read the Docs webhook from there.</p>
<p>To continue building your Read the Docs project on changes to your repository, you will need to add a new webhook on your GitHub repository. You can either connect your GitHub account and configure a GitHub webhook integration, or you can add a generic webhook integration.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do wonder if the instructions about what the user has to do are clear and simple. I have a feeling that we are pointing the users to read more docs from the email instead of doing something actionable like: "Resync your webhook".

Maybe I'm wrong, but shouldn't work directly if the user goes to Admin -> Integrations -> Github -> Resync webhook. If that's the case, we could also generate the link to that section here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the user might not even have an integration set up if they used GitHub Services. So there would be nothing to resync. Probably easiest to just give them full directions.

@agjohnson agjohnson merged commit fb4bd86 into humitos/builds/notify-old-endpoints Jan 8, 2019
@agjohnson agjohnson deleted the agj/notify-old-endpoints branch January 8, 2019 17:52
agjohnson pushed a commit that referenced this pull request Jan 11, 2019
* Notify users about the usage of deprecated webhooks

Each time a deprecated webhook is hit, a notification is
created (without duplicating it) to be sent.

* Extend notification to support de-dup and delayed email sent

* Improve decorator to support generic and specific VCS webhook views

* Remove no necessary settings

* DeprecatedWebhookEndpointNotification tests and improvements

* Better docstring

* Lint

* Update copy on notifications for github services deprecation (#5067)

* Updated copy on webhooks

* Drop "deprecated webhook endpoint" copy, this is core team nomenclature, not user
nomenclature.
* Add small amount of docs to point to

* Update docs and point to docs in notification message

* Add year

* Split up deprecated view notification to GitHub and other webhook endpoints (#5083)

* Updated copy on webhooks

* Drop "deprecated webhook endpoint" copy, this is core team nomenclature, not user
nomenclature.
* Add small amount of docs to point to

* Update docs and point to docs in notification message

* Split up deprecated view notification to GitHub and other webhook endpoints

This sets a date for deprecated of these endpoints as Mar 1st 2019. Too
soon?

* Reduce complexity and drop decorator pattern for Notification
  classmethod pattern used in other notifications
* Add notifications for non-GitHub incoming webhooks
* Add docs as well

* More renaming and slight refactor

Found out 2x messages are being generated, so this stops the automated
mechanism for triggering these messages.

* Update dates

* Also update docs

* Typo on date

* Back out some more of the changes to notifications to make them operable without automation

* Add admin method for notification

* Add admin filter for project features
agjohnson pushed a commit that referenced this pull request Jan 11, 2019
* Notify users about the usage of deprecated webhooks

Each time a deprecated webhook is hit, a notification is
created (without duplicating it) to be sent.

* Extend notification to support de-dup and delayed email sent

* Improve decorator to support generic and specific VCS webhook views

* Remove no necessary settings

* DeprecatedWebhookEndpointNotification tests and improvements

* Better docstring

* Lint

* Update copy on notifications for github services deprecation (#5067)

* Updated copy on webhooks

* Drop "deprecated webhook endpoint" copy, this is core team nomenclature, not user
nomenclature.
* Add small amount of docs to point to

* Update docs and point to docs in notification message

* Add year

* Split up deprecated view notification to GitHub and other webhook endpoints (#5083)

* Updated copy on webhooks

* Drop "deprecated webhook endpoint" copy, this is core team nomenclature, not user
nomenclature.
* Add small amount of docs to point to

* Update docs and point to docs in notification message

* Split up deprecated view notification to GitHub and other webhook endpoints

This sets a date for deprecated of these endpoints as Mar 1st 2019. Too
soon?

* Reduce complexity and drop decorator pattern for Notification
  classmethod pattern used in other notifications
* Add notifications for non-GitHub incoming webhooks
* Add docs as well

* More renaming and slight refactor

Found out 2x messages are being generated, so this stops the automated
mechanism for triggering these messages.

* Update dates

* Also update docs

* Typo on date

* Back out some more of the changes to notifications to make them operable without automation

* Add admin method for notification

* Add admin filter for project features
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants